Skip to content

fix: hook error sanitize/dedup + abort trailer tool-call mention + token parity - #66

Merged
yogthos merged 1 commit into
mainfrom
chore/r7-followup-bugs
May 21, 2026
Merged

fix: hook error sanitize/dedup + abort trailer tool-call mention + token parity#66
yogthos merged 1 commit into
mainfrom
chore/r7-followup-bugs

Conversation

@yogthos

@yogthos yogthos commented May 21, 2026

Copy link
Copy Markdown
Collaborator

TDD round of fixes for bugs found auditing PRs #64 and #65. Multi-line Janet errors corrupted the notif wire format; consecutive identical hook errors flooded the chat (a buggy on-message-update could push thousands per response). Partial-on-abort trailer now warns when tool calls ran but their results aren't in the preserved text — next turn's LLM context no longer treats the partial as definitive. Token accumulator parity on abort path. 6 new tests, 622 pass total.

…t tool count + token parity

TDD: tests first, all 6 new tests failed initially, then implemented.

## Round A — hook error sanitize + dedup

Two bugs in PR #64's hook-error notification path:

1. Multi-line / tab-containing Janet errors broke the
   `level\tmsg\n` wire format. A `(error "trace\n  at file:42")`
   produced multiple malformed notification entries (one per source
   line), the first with truncated content and the rest filtered
   out as malformed. drain_notifications splits raw on `\n` per
   entry and on first `\t` per level/msg — both control chars now
   sanitized in Janet before push.

2. A buggy `on-message-update` hook (fires ~every 16 streamed
   tokens) flooded the chat with thousands of identical "[plugin]
   hook X.Y errored: ..." banners during a single long response.
   Now deduped: two new Janet vars track the most-recent sanitized
   error msg + a consecutive-repeat count; identical errors just
   bump the count instead of pushing. On drain, any outstanding
   count is flushed as a "(repeated N times)" summary entry.

Implementation:
- `harness/sanitize-hook-err` (new) normalizes `\t` → space and
  `\n`/`\r\n` → ` | `. Distinct hook errors stay separate; only
  consecutive identical ones collapse. Wrote with explicit nested
  `string/replace-all` calls — Janet's `->` threading macro
  would pass the string in the wrong arg position
  (string/replace-all expects `(patt subst str)`).
- `harness/push-hook-err` (new) does the dedup check using
  `harness-last-hook-err-msg` + `harness-last-hook-err-count`
  module-level vars.
- `drain_notifications` flushes pending dedup count before reading
  the notif list so a 50× repeat shows up as a single
  "(repeated 50 times)" entry in the next drain.
- The catch arm in `dispatch` calls these instead of appending
  directly. Wrapped in explicit `(do ...)` for Janet's
  single-form catch-body semantics.

## Round B — partial-on-abort trailer notes tool calls

PR #65 saved the streamed assistant text on abort but didn't
indicate that tool calls had also run in the same turn (whose
results aren't in `response_buf` — only Token events accumulate
there). The LLM on next turn would see the partial as a definitive
"this was my reply" and could re-run side-effecting tools.

`capture_partial_on_abort` now takes a `tool_calls_in_turn: u32`
parameter. When non-zero, the trailer reads:
  [interrupted by user (Ctrl+C); 2 tool calls ran in this turn — results not preserved]
Singular case ("1 tool call ran") uses the right noun.

UI loop tracks `tool_calls_this_run: u32`, incremented on every
`AgentEvent::ToolCall`, reset on `Done`/`Interjected`/both abort
sites (since each marks the end of one agent run).

## Round C — token-accumulator parity on abort

`Done` and `Interjected` branches both update `session.total_tokens`
alongside the message add. The abort path didn't — made aborted
turns look like zero-token contributions in the placeholder
field. Fixed with an explicit
`session.total_tokens.saturating_add(Session::estimate_tokens(&stashed))`
inside `capture_partial_on_abort`.

Both fields stay under the `TODO(cost-tracking)` comment but at
least they're now internally consistent.

## Test plan

- [x] 6 new tests (3 plugin dispatch + 3 capture_partial_on_abort
      + 2 updated existing tests with new signature).
- [x] `cargo test --features plugin` -> 622 pass, 0 fail.
- [x] `cargo build --all-features` -> compiles.

## Skipped (observational, not bugs)

- #3 print/loop mode notifications never drained: print mode is
  non-interactive; tracing::warn (via `--verbose`) is the right
  channel.
- #4 Janet `err` non-string-coerced: `(string ...)` calls Janet's
  `tostring` which handles any value type. Documented behavior.
- #7 markdown rendering of `[interrupted by user (Ctrl+C)]`: not
  a link by pulldown-cmark's rules; visually acceptable inline.
@yogthos
yogthos merged commit e5ba6d2 into main May 21, 2026
1 check passed
@yogthos
yogthos deleted the chore/r7-followup-bugs branch May 21, 2026 01:48
allen-munsch pushed a commit to allen-munsch/dirge that referenced this pull request Jun 3, 2026
…t tool count + token parity (dirge-code#66)

TDD: tests first, all 6 new tests failed initially, then implemented.

## Round A — hook error sanitize + dedup

Two bugs in PR dirge-code#64's hook-error notification path:

1. Multi-line / tab-containing Janet errors broke the
   `level\tmsg\n` wire format. A `(error "trace\n  at file:42")`
   produced multiple malformed notification entries (one per source
   line), the first with truncated content and the rest filtered
   out as malformed. drain_notifications splits raw on `\n` per
   entry and on first `\t` per level/msg — both control chars now
   sanitized in Janet before push.

2. A buggy `on-message-update` hook (fires ~every 16 streamed
   tokens) flooded the chat with thousands of identical "[plugin]
   hook X.Y errored: ..." banners during a single long response.
   Now deduped: two new Janet vars track the most-recent sanitized
   error msg + a consecutive-repeat count; identical errors just
   bump the count instead of pushing. On drain, any outstanding
   count is flushed as a "(repeated N times)" summary entry.

Implementation:
- `harness/sanitize-hook-err` (new) normalizes `\t` → space and
  `\n`/`\r\n` → ` | `. Distinct hook errors stay separate; only
  consecutive identical ones collapse. Wrote with explicit nested
  `string/replace-all` calls — Janet's `->` threading macro
  would pass the string in the wrong arg position
  (string/replace-all expects `(patt subst str)`).
- `harness/push-hook-err` (new) does the dedup check using
  `harness-last-hook-err-msg` + `harness-last-hook-err-count`
  module-level vars.
- `drain_notifications` flushes pending dedup count before reading
  the notif list so a 50× repeat shows up as a single
  "(repeated 50 times)" entry in the next drain.
- The catch arm in `dispatch` calls these instead of appending
  directly. Wrapped in explicit `(do ...)` for Janet's
  single-form catch-body semantics.

## Round B — partial-on-abort trailer notes tool calls

PR dirge-code#65 saved the streamed assistant text on abort but didn't
indicate that tool calls had also run in the same turn (whose
results aren't in `response_buf` — only Token events accumulate
there). The LLM on next turn would see the partial as a definitive
"this was my reply" and could re-run side-effecting tools.

`capture_partial_on_abort` now takes a `tool_calls_in_turn: u32`
parameter. When non-zero, the trailer reads:
  [interrupted by user (Ctrl+C); 2 tool calls ran in this turn — results not preserved]
Singular case ("1 tool call ran") uses the right noun.

UI loop tracks `tool_calls_this_run: u32`, incremented on every
`AgentEvent::ToolCall`, reset on `Done`/`Interjected`/both abort
sites (since each marks the end of one agent run).

## Round C — token-accumulator parity on abort

`Done` and `Interjected` branches both update `session.total_tokens`
alongside the message add. The abort path didn't — made aborted
turns look like zero-token contributions in the placeholder
field. Fixed with an explicit
`session.total_tokens.saturating_add(Session::estimate_tokens(&stashed))`
inside `capture_partial_on_abort`.

Both fields stay under the `TODO(cost-tracking)` comment but at
least they're now internally consistent.

## Test plan

- [x] 6 new tests (3 plugin dispatch + 3 capture_partial_on_abort
      + 2 updated existing tests with new signature).
- [x] `cargo test --features plugin` -> 622 pass, 0 fail.
- [x] `cargo build --all-features` -> compiles.

## Skipped (observational, not bugs)

- #3 print/loop mode notifications never drained: print mode is
  non-interactive; tracing::warn (via `--verbose`) is the right
  channel.
- #4 Janet `err` non-string-coerced: `(string ...)` calls Janet's
  `tostring` which handles any value type. Documented behavior.
- dirge-code#7 markdown rendering of `[interrupted by user (Ctrl+C)]`: not
  a link by pulldown-cmark's rules; visually acceptable inline.

Co-authored-by: Yogthos <yogthos@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant